Async waitForNavigation #467

inancgumus · 2022-07-21T14:25:08Z

Please feel free to add additional commits to the PR if you need to and merge it without me.

Note: I would create another PR to fix the problem in k6ext.promise, but since I'll be leaving soon, I wanted to pack it in the same PR.

Closes #440.

examples/fillform.js

imiric

LGTM, besides the test and examples issues. The async helpers are a nice touch 👍

And a meta comment: some of the commits here could be squashed.

examples/locator_pom.js

tests/frame_manager_test.go

ankur22 · 2022-07-28T14:20:19Z

LGTM.

@imiric you raise good points on the tests. I also think we should be checking that reject is called when the javascript test fails.

The callback returned from RegisterCallback should not be run as a goroutine. Since its job is to carry a task to the event loop, the task should be done in a goroutine first, and then let the task call the callback within the goroutine.

Since waitForNavigation is async now.

Since the Click and WaitForNavigation are now async, we don't need to call each in separate goroutines. Instead, we call them in the event loop, and the event loop will take care of async stuff for us.

We can now use Promise.all when we interact with an element that causes a navigation to occur, and so we need to also use waitForNavigation before carrying on to the next steps in the tests. This helps prevent race conditions between clicking and waiting. Resolves: #467 (comment)

The test now checks the state of the promise before passing the test. Resolves: #467 (comment)

Resolves: #467 (comment)

We just want to make sure that the promises actually complete and are fulfilled before we end the test, so we're checking the state of the promise in `tb.await` as well as after `tb.await`. There's a chance that the action has already completed before the first state check, but the resolve/rejection only occurs `tb.await` returns. Resolves: #467 (comment)

k6ext/k6test/vu.go

imiric

Looks good, thanks for the changes @ankur22. I left just a few more minor suggestions.

tests/frame_manager_test.go

k6ext/k6test/vu.go

We just want to make sure that the promises actually complete and are fulfilled before we end the test, so we're checking the state of the promise in `tb.await` as well as after `tb.await`. There's a chance that the action has already completed before the first state check, but the resolve/rejection only occurs `tb.await` returns. Resolves: #467 (comment)

This will ensure that the envetloop has complete before starting it up again. We're going to avoid placing lock here, since we want to follow the eventloop API and not allow multiple threads accessing the one eventloop.

We shouldn't need the CI specific timeout since the test should now be less flaky and is asynchronous. Resolves: #467 (comment)

tests/frame_manager_test.go

imiric

Great work @inancgumus and @ankur22! 👏

inancgumus requested review from imiric and ankur22 July 21, 2022 14:25

inancgumus self-assigned this Jul 21, 2022

This was referenced Jul 21, 2022

Async WaitForNavigation #440

Closed

Data race in event loop grafana/k6#2612

Closed

inancgumus force-pushed the feat/440-async-waitfornavigation branch 4 times, most recently from 61d50a4 to 39fbbf0 Compare July 22, 2022 14:24

imiric reviewed Jul 25, 2022

View reviewed changes

examples/fillform.js Outdated Show resolved Hide resolved

imiric added the breaking PRs that need to be mentioned in the breaking changes section of the release notes label Jul 27, 2022

imiric suggested changes Jul 28, 2022

View reviewed changes

examples/locator_pom.js Outdated Show resolved Hide resolved

tests/frame_manager_test.go Show resolved Hide resolved

tests/frame_manager_test.go Outdated Show resolved Hide resolved

tests/frame_manager_test.go Outdated Show resolved Hide resolved

inancgumus added 12 commits August 8, 2022 10:33

Fix k6ext.promise

a7c68ac

The callback returned from RegisterCallback should not be run as a goroutine. Since its job is to carry a task to the event loop, the task should be done in a goroutine first, and then let the task call the callback within the goroutine.

Refactor FrameManager.WaitForFrameNavigation

8c17ed2

Make api.Frame.WaitForNavigation async

c02e51e

Make api.Page.WaitForNavigation async

56782c5

Make Frame.WaitForNavigation async

7ccd24c

Make Page.WaitForNavigation async

1639294

Make fillform.js async for waitForNavigation

7cb311c

Make locator.js async for waitForNavigation

2b824cb

Make locator_pom.js async for waitForNavigation

f2db16d

Make README locator example async

301e746

Since waitForNavigation is async now.

Refactor TestWaitForFrameNavigationWithinDocument

59bed1e

Since the Click and WaitForNavigation are now async, we don't need to call each in separate goroutines. Instead, we call them in the event loop, and the event loop will take care of async stuff for us.

Add TestWaitForFrameNavigation

41ae327

ankur22 force-pushed the feat/440-async-waitfornavigation branch from 39fbbf0 to 41ae327 Compare August 8, 2022 09:36

ankur22 force-pushed the feat/440-async-waitfornavigation branch from 8a9f052 to 886eda5 Compare August 8, 2022 11:55

ankur22 added a commit that referenced this pull request Aug 8, 2022

Refactor test to check promise states

d37be91

The test now checks the state of the promise before passing the test. Resolves: #467 (comment)

ankur22 added a commit that referenced this pull request Aug 8, 2022

Refactor test to check promise states

a68a949

The test now checks the state of the promise before passing the test. Resolves: #467 (comment)

ankur22 force-pushed the feat/440-async-waitfornavigation branch from d37be91 to a68a949 Compare August 8, 2022 16:40

Refactor test to check promise states

331465f

The test now checks the state of the promise before passing the test. Resolves: #467 (comment)

ankur22 force-pushed the feat/440-async-waitfornavigation branch from a68a949 to 331465f Compare August 8, 2022 16:58

Remove extra promise handler chaining

e118dc3

Resolves: #467 (comment)

ankur22 force-pushed the feat/440-async-waitfornavigation branch from 17f107f to 262eaee Compare August 9, 2022 10:32

ankur22 requested a review from imiric August 9, 2022 10:33

ankur22 reviewed Aug 9, 2022

View reviewed changes

k6ext/k6test/vu.go Show resolved Hide resolved

imiric suggested changes Aug 10, 2022

View reviewed changes

ankur22 force-pushed the feat/440-async-waitfornavigation branch from e63220b to 8b23121 Compare August 11, 2022 15:17

ankur22 added 2 commits August 11, 2022 16:23

Add WaitOnRegistered before Starting eventloop

61b84ee

This will ensure that the envetloop has complete before starting it up again. We're going to avoid placing lock here, since we want to follow the eventloop API and not allow multiple threads accessing the one eventloop.

ankur22 force-pushed the feat/440-async-waitfornavigation branch from 8b23121 to 61b84ee Compare August 11, 2022 15:23

ankur22 added a commit that referenced this pull request Aug 11, 2022

Remove the CI specific timeout

b27f684

We shouldn't need the CI specific timeout since the test should now be less flaky and is asynchronous. Resolves: #467 (comment)

ankur22 added 2 commits August 11, 2022 16:46

Remove the CI specific timeout

d7e15c5

We shouldn't need the CI specific timeout since the test should now be less flaky and is asynchronous. Resolves: #467 (comment)

Add timeout to WaitForNavigation

fac54bd

ankur22 force-pushed the feat/440-async-waitfornavigation branch from 1e9fda0 to fac54bd Compare August 11, 2022 15:46

ankur22 requested a review from imiric August 11, 2022 16:12

imiric reviewed Aug 12, 2022

View reviewed changes

tests/frame_manager_test.go Outdated Show resolved Hide resolved

Add timeout to Goto

3ea0c25

ankur22 force-pushed the feat/440-async-waitfornavigation branch from f510cff to 3ea0c25 Compare August 12, 2022 08:41

ankur22 requested a review from imiric August 12, 2022 08:46

imiric approved these changes Aug 12, 2022

View reviewed changes

ankur22 merged commit c7b281b into main Aug 12, 2022

ankur22 mentioned this pull request Aug 12, 2022

Expand TestWaitForFrameNavigationWithinDocument to perform a functionality check grafana/k6#4424

Open

imiric mentioned this pull request Jan 30, 2025

Add ElementHandle.isInViewport() grafana/k6#4423

Open

imiric deleted the feat/440-async-waitfornavigation branch August 12, 2022 09:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Async waitForNavigation #467

Async waitForNavigation #467

inancgumus commented Jul 21, 2022 •

edited

Loading

imiric left a comment •

edited

Loading

ankur22 commented Jul 28, 2022 •

edited

Loading

imiric left a comment

imiric left a comment

Async waitForNavigation #467

Async waitForNavigation #467

Conversation

inancgumus commented Jul 21, 2022 • edited Loading

imiric left a comment • edited Loading

Choose a reason for hiding this comment

ankur22 commented Jul 28, 2022 • edited Loading

imiric left a comment

Choose a reason for hiding this comment

imiric left a comment

Choose a reason for hiding this comment

inancgumus commented Jul 21, 2022 •

edited

Loading

imiric left a comment •

edited

Loading

ankur22 commented Jul 28, 2022 •

edited

Loading